Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: added theme switcher with match-system option #1273

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kirill-dev-pro
Copy link

image image

@nlopin
Copy link
Collaborator

nlopin commented Nov 26, 2024

привет! а мы не можем то же самое на Vue сделать? Я не помню, чтобы мы WebComponents использовали во фронтенде и если это первый случай, я бы предпочел не размывать тех.стек и сделать все на вью.

Copy link
Contributor

@trin4ik trin4ik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Спасибо за PR, многое из ревью из категории "разрешите доеб*ться"

@@ -9,12 +9,6 @@
{% block meta %}
{% include "common/meta.html" %}
{% endblock %}
<script>
const theme = localStorage.getItem('theme');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В некоторых местах далее проверяется typeof localStorage !== 'undefined' перед юзанием LS. Предлагаю что тут, что далее юзать nullsafe, т.е. вместо проверок и дерева из if, просто юзать

const theme = localStorage?.getItem('theme') ?? 'auto'

theme === 'auto' || theme === 'dark' || theme === 'light' ? theme : 'auto';

const loadTheme = () =>
parseTheme(typeof localStorage !== 'undefined' && localStorage.getItem(storageKey));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Выше уже писал, предлагаю юзать nullsafe

parseTheme(localStorage?.getItem(storageKey));

Comment on lines +22 to +26
function storeTheme(theme) {
if (typeof localStorage !== 'undefined') {
localStorage.setItem(storageKey, theme === 'light' || theme === 'dark' ? theme : '');
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

зачем это в глобальной видимости?

matchMedia('(prefers-color-scheme: light)').matches ? 'light' : 'dark';

matchMedia(`(prefers-color-scheme: light)`).addEventListener('change', () => {
if (loadTheme() === 'auto') onThemeChange('auto');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onThemeChange метод находится в классе StarlightThemeSelect, это просто не работает и вызывает ошибку.

constructor() {
const theme = this.storedTheme ||
(window.matchMedia('(prefers-color-scheme: light)').matches ? 'light' : 'dark');
document.documentElement.dataset.theme = theme === 'light' ? 'light' : 'dark';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dataset оперирует data- аттрибутами, в данном случае как я понял предполагается поменять theme аттрибут, а не data-theme.


updatePickers(theme = this.storedTheme || 'auto') {
document.querySelectorAll('starlight-theme-select').forEach((picker) => {
console.log('picker', picker);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Надо бы кильнуть, пакер не убирает логи из продакшен-бандла

constructor() {
super();
this.onThemeChange(loadTheme());
this.querySelector('select')?.addEventListener('change', (e) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Зачем тут nullsafe?

super();
this.onThemeChange(loadTheme());
this.querySelector('select')?.addEventListener('change', (e) => {
if (e.currentTarget instanceof HTMLSelectElement) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

событие change, прослушиваемое на select теге всегда же будет возвращать этот самый селект как target, зачем проверять?

Comment on lines +51 to +58
const tmpl = document.querySelector(`#theme-icons`);
const newIcon = tmpl && tmpl.content.querySelector('.' + theme);
if (newIcon) {
const oldIcon = picker.querySelector('starlight-theme-select i.label-icon');
if (oldIcon) {
oldIcon.replaceChildren(...newIcon.cloneNode(true).childNodes);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Owner

@vas3k vas3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Минус эстетика это минус вайб! Надо подумать как текущий контрол адаптировать под три положения, а не пытаться заменить на старый советский селект :)

@kirill-dev-pro
Copy link
Author

@vas3k Текущий контрол это checkbox, у него по определению не может быть 3 положений

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants